planner: support index_join_first() hint to prefer index join#66596
planner: support index_join_first() hint to prefer index join#66596fixdb wants to merge 3 commits intopingcap:masterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-tests-checked
release-note
|
Review Complete Findings: 1 issues |
ti-chi-bot
bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @fixdb. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
📝 WalkthroughWalkthroughAdds a new statement-level optimizer hint Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant Planner
participant Physical
Client->>Parser: SQL with INDEX_JOIN_FIRST hint
Parser->>Parser: tokenize/recognize hint -> PlanHints(IndexJoinFirst=true)
Parser->>Planner: AST + PlanHints
Planner->>Planner: set PreferIndexJoinFirst in join hint flags
Planner->>Physical: exhaust_physical_plans(with PreferIndexJoinFirst)
Physical->>Physical: preferIndexJoinFamily favors index-join variants
Physical-->>Planner: chosen physical plan (index-join when applicable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/util/hint/hint.go (1)
132-134: Align wording with actual soft-preference semantics.The comment says “always prefer”, but the implementation and surrounding docs describe a “when possible” preference.
✏️ Suggested wording tweak
- // HintIndexJoinFirst is a SQL hint to indicate the optimizer should always prefer index join over other join types. + // HintIndexJoinFirst is a SQL hint to indicate the optimizer should prefer index join over other join types when possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/hint/hint.go` around lines 132 - 134, Update the doc comment for the HintIndexJoinFirst constant to reflect its soft-preference semantics (i.e., prefer index join when possible rather than "always prefer"); modify the comment above HintIndexJoinFirst to say it indicates the optimizer should prefer index joins when feasible and that it does not force them or take parameters, keeping the constant name and behavior unchanged so readers understand it is a best-effort preference.pkg/planner/core/operator/logicalop/logical_join.go (1)
1710-1715: Alignindex_join_firstassignment with its stated precedence contractOn Line 1713, the code sets
PreferIndexJoinFirstunconditionally, while the comment says it should be set only when no side-specific index-join hint is forced. Adding that guard here would keep precedence explicit and less brittle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/operator/logicalop/logical_join.go` around lines 1710 - 1715, The assignment of p.PreferJoinType |= utilhint.PreferIndexJoinFirst when hintInfo.IndexJoinFirst is true should be guarded so it only applies if no side-specific index-join hint is forced; update the block around hintInfo.IndexJoinFirst to check that neither left nor right table-specific index-join force flags are set (e.g., the corresponding hintInfo fields that indicate a forced index join for a particular side) before setting p.PreferJoinType, leaving other behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/casetest/hint/hint_test.go`:
- Around line 456-521: The test's plan-type matcher only treats "IndexJoin" and
"IndexHashJoin" as valid, causing false failures when plans use
"IndexMergeJoin"; update each check that sets hasIndexJoin by expanding the
condition to also accept "IndexMergeJoin" (look for occurrences of hasIndexJoin
variable and the planType checks in hint_test.go) so the test recognizes all
index-join-family plans as success.
In `@pkg/planner/core/exhaust_physical_plans.go`:
- Around line 2272-2276: The current block in the INDEX_JOIN_FIRST path returns
true for any index-join family before honoring explicit INL_* preferences;
change the logic in the p.PreferAny(h.PreferIndexJoinFirst) branch to first call
getIndexJoinSideAndMethod(physicPlan) to determine isIndexJoin, and only return
true when isIndexJoin is true AND none of the explicit INL preferences are set
(i.e. !p.PreferAny(h.PreferInlJoin, h.PreferInlHashJoin, h.PreferInlMergeJoin));
otherwise fall through so explicit INL_JOIN/INL_HASH_JOIN/INL_MERGE_JOIN
preferences can still decide.
---
Nitpick comments:
In `@pkg/planner/core/operator/logicalop/logical_join.go`:
- Around line 1710-1715: The assignment of p.PreferJoinType |=
utilhint.PreferIndexJoinFirst when hintInfo.IndexJoinFirst is true should be
guarded so it only applies if no side-specific index-join hint is forced; update
the block around hintInfo.IndexJoinFirst to check that neither left nor right
table-specific index-join force flags are set (e.g., the corresponding hintInfo
fields that indicate a forced index join for a particular side) before setting
p.PreferJoinType, leaving other behavior unchanged.
In `@pkg/util/hint/hint.go`:
- Around line 132-134: Update the doc comment for the HintIndexJoinFirst
constant to reflect its soft-preference semantics (i.e., prefer index join when
possible rather than "always prefer"); modify the comment above
HintIndexJoinFirst to say it indicates the optimizer should prefer index joins
when feasible and that it does not force them or take parameters, keeping the
constant name and behavior unchanged so readers understand it is a best-effort
preference.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
pkg/parser/ast/misc.gopkg/parser/hintparser.gopkg/parser/hintparser.ypkg/parser/misc.gopkg/planner/core/casetest/hint/hint_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/rule_join_reorder.gopkg/util/hint/hint.go
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #66596 +/- ##
================================================
- Coverage 77.6584% 77.5452% -0.1133%
================================================
Files 2008 2009 +1
Lines 549335 549429 +94
================================================
- Hits 426605 426056 -549
- Misses 121059 121665 +606
- Partials 1671 1708 +37
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/util/hint/hint.go (1)
966-967: Consider adding duplicate hint warning for consistency.Other hints in this function (e.g.,
straightJoinHintCnt) track multiple occurrences and emit warnings when defined more than once. Theindex_join_firsthint doesn't track or warn about duplicates. While this is a minor issue since the hint is idempotent (settingtruemultiple times has no effect), adding a counter and warning would be consistent with the existing pattern.♻️ Optional: Add duplicate hint warning
// Add counter near other hint counters (around line 794) var indexJoinFirstHintCnt int // In the switch case: case HintIndexJoinFirst: indexJoinFirstHintCnt++ indexJoinFirst = true // After parsing loop, add warning check: if indexJoinFirstHintCnt > 1 { warnHandler.SetHintWarning("INDEX_JOIN_FIRST() is defined more than once, only the last definition takes effect") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/hint/hint.go` around lines 966 - 967, The HintIndexJoinFirst branch currently sets indexJoinFirst = true but does not track duplicates; add an integer counter (e.g., indexJoinFirstHintCnt) alongside the other hint counters, increment it in the case for HintIndexJoinFirst, and after parsing check if indexJoinFirstHintCnt > 1 and call warnHandler.SetHintWarning with a message like "INDEX_JOIN_FIRST() is defined more than once, only the last definition takes effect" to mirror the duplicate-hint behavior used for straightJoinHintCnt and others.pkg/planner/core/operator/logicalop/logical_join.go (1)
1710-1715: Comment and behavior are slightly out of sync.The comment says the flag is set only when no side is forced, but the code sets it unconditionally and defers precedence handling downstream. Consider aligning the comment to avoid future confusion.
✏️ Suggested comment-only adjustment
- // index_join_first is a statement-level hint that prefers index join on all joins. - // It is applied after table-specific hints; if a table-specific index join hint was set, - // it is already covered. We only set the flag here when no specific side is forced. + // index_join_first is a statement-level hint that prefers index join on all joins. + // It is applied after table-specific hints. Precedence vs. explicit INL_* hints + // is resolved downstream during physical-plan preference checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/operator/logicalop/logical_join.go` around lines 1710 - 1715, The comment above the index_join_first handling is inaccurate: it claims the flag is set only when no side is forced, but the code unconditionally sets p.PreferJoinType |= utilhint.PreferIndexJoinFirst when hintInfo.IndexJoinFirst is true and relies on downstream precedence resolution; update the comment to state that IndexJoinFirst is applied here regardless of side and that precedence between table-specific hints and this statement-level hint is resolved later (mentioning hintInfo.IndexJoinFirst, p.PreferJoinType, and utilhint.PreferIndexJoinFirst).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/casetest/hint/hint_test.go`:
- Around line 526-548: The current checkPlan helper only asserts the operator
kind and can miss whether the chosen inner side matches the
INL_JOIN(t1)/INL_HASH_JOIN(t1) hint; modify checkPlan (or add a new helper used
in these cases) to also assert the inner table is t1 by checking the plan row
text for the inner-side marker (e.g., ensure the plan string contains both the
operator name "IndexJoin"/"IndexHashJoin" and an explicit inner reference to
"t1" or the planner's inner-side notation), then update the two calls that
assert INL_JOIN(t1) and INL_HASH_JOIN(t1) to use this tightened check so both
operator and inner table are validated.
---
Nitpick comments:
In `@pkg/planner/core/operator/logicalop/logical_join.go`:
- Around line 1710-1715: The comment above the index_join_first handling is
inaccurate: it claims the flag is set only when no side is forced, but the code
unconditionally sets p.PreferJoinType |= utilhint.PreferIndexJoinFirst when
hintInfo.IndexJoinFirst is true and relies on downstream precedence resolution;
update the comment to state that IndexJoinFirst is applied here regardless of
side and that precedence between table-specific hints and this statement-level
hint is resolved later (mentioning hintInfo.IndexJoinFirst, p.PreferJoinType,
and utilhint.PreferIndexJoinFirst).
In `@pkg/util/hint/hint.go`:
- Around line 966-967: The HintIndexJoinFirst branch currently sets
indexJoinFirst = true but does not track duplicates; add an integer counter
(e.g., indexJoinFirstHintCnt) alongside the other hint counters, increment it in
the case for HintIndexJoinFirst, and after parsing check if
indexJoinFirstHintCnt > 1 and call warnHandler.SetHintWarning with a message
like "INDEX_JOIN_FIRST() is defined more than once, only the last definition
takes effect" to mirror the duplicate-hint behavior used for straightJoinHintCnt
and others.
ℹ️ Review info
Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e59936aa-5118-42bf-8f14-015f52deecc5
📒 Files selected for processing (9)
pkg/parser/ast/misc.gopkg/parser/hintparser.gopkg/parser/hintparser.ypkg/parser/misc.gopkg/planner/core/casetest/hint/hint_test.gopkg/planner/core/exhaust_physical_plans.gopkg/planner/core/operator/logicalop/logical_join.gopkg/planner/core/rule_join_reorder.gopkg/util/hint/hint.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/parser/hintparser.y
| checkPlan := func(query, wantOp string, msg string) { | ||
| rows = testKit.MustQuery(query).Rows() | ||
| found := false | ||
| for _, row := range rows { | ||
| if strings.Contains(row[0].(string), wantOp) { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| require.True(t, found, msg) | ||
| } | ||
| // INL_JOIN(t1): must choose IndexJoin (not IndexHashJoin) with t1 as inner. | ||
| checkPlan( | ||
| "explain format='brief' select /*+ INDEX_JOIN_FIRST() INL_JOIN(t1) */ t1.a, t2.b from t1 join t2 on t1.a = t2.a", | ||
| "IndexJoin", | ||
| "INL_JOIN(t1) should win over INDEX_JOIN_FIRST(): IndexJoin with t1 as inner required", | ||
| ) | ||
| // INL_HASH_JOIN(t1): must choose IndexHashJoin (not plain IndexJoin) with t1 as inner. | ||
| checkPlan( | ||
| "explain format='brief' select /*+ INDEX_JOIN_FIRST() INL_HASH_JOIN(t1) */ t1.a, t2.b from t1 join t2 on t1.a = t2.a", | ||
| "IndexHashJoin", | ||
| "INL_HASH_JOIN(t1) should win over INDEX_JOIN_FIRST(): IndexHashJoin with t1 as inner required", | ||
| ) |
There was a problem hiding this comment.
INL_* regression checks currently miss inner-side validation.
checkPlan only checks the operator kind. It can still pass when the chosen inner side does not match INL_JOIN(t1) / INL_HASH_JOIN(t1) requirements.
✅ Tighten regression intent (method + target table)
- checkPlan := func(query, wantOp string, msg string) {
+ checkPlan := func(query, wantOp, wantInnerTbl string, msg string) {
rows = testKit.MustQuery(query).Rows()
found := false
for _, row := range rows {
- if strings.Contains(row[0].(string), wantOp) {
+ planType := row[0].(string)
+ rowText := strings.ToLower(strings.TrimSpace(strings.Join([]string{
+ planType,
+ row[len(row)-1].(string),
+ }, " ")))
+ if strings.Contains(planType, wantOp) && strings.Contains(rowText, strings.ToLower(wantInnerTbl)) {
found = true
break
}
}
require.True(t, found, msg)
}
@@
- checkPlan(
+ checkPlan(
"explain format='brief' select /*+ INDEX_JOIN_FIRST() INL_JOIN(t1) */ t1.a, t2.b from t1 join t2 on t1.a = t2.a",
"IndexJoin",
+ "t1",
"INL_JOIN(t1) should win over INDEX_JOIN_FIRST(): IndexJoin with t1 as inner required",
)
@@
- checkPlan(
+ checkPlan(
"explain format='brief' select /*+ INDEX_JOIN_FIRST() INL_HASH_JOIN(t1) */ t1.a, t2.b from t1 join t2 on t1.a = t2.a",
"IndexHashJoin",
+ "t1",
"INL_HASH_JOIN(t1) should win over INDEX_JOIN_FIRST(): IndexHashJoin with t1 as inner required",
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/casetest/hint/hint_test.go` around lines 526 - 548, The
current checkPlan helper only asserts the operator kind and can miss whether the
chosen inner side matches the INL_JOIN(t1)/INL_HASH_JOIN(t1) hint; modify
checkPlan (or add a new helper used in these cases) to also assert the inner
table is t1 by checking the plan row text for the inner-side marker (e.g.,
ensure the plan string contains both the operator name
"IndexJoin"/"IndexHashJoin" and an explicit inner reference to "t1" or the
planner's inner-side notation), then update the two calls that assert
INL_JOIN(t1) and INL_HASH_JOIN(t1) to use this tightened check so both operator
and inner table are validated.
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/rule_join_reorder.go (1)
723-728: PotentialHintInfooverwrite when combining table-specific and statement-level hints.
SetNewJoinWithHint(line 722) may setjoin.HintInfofromjoinMethodHintInfofor table-specific hints. Then line 727 unconditionally overwrites it withindexJoinFirstHints. If a query uses bothINDEX_JOIN_FIRST()and table-specific hints likeHASH_JOIN(t1), the table-specificHintInfocontext is lost, which could affect warning generation for unused hints.Consider merging hint info rather than overwriting, or at minimum documenting this precedence behavior. That said, the existing
SetNewJoinWithHintalready exhibits an overwrite pattern between left/right children, so this follows established behavior.Potential improvement (optional)
If both hint sources should be preserved:
if s.indexJoinFirstHints != nil { join.PreferJoinType |= h.PreferIndexJoinFirst - join.HintInfo = s.indexJoinFirstHints + // Only set HintInfo if not already set by table-specific hints + if join.HintInfo == nil { + join.HintInfo = s.indexJoinFirstHints + } }Alternatively, document the intentional precedence if INDEX_JOIN_FIRST should take priority.
ℹ️ Review info
Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d431fa77-5fe7-4dd4-9388-1c350f66c615
📒 Files selected for processing (1)
pkg/planner/core/rule_join_reorder.go
|
@fixdb: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: close #61501
Problem Summary:
What changed and how does it work?
support index_join_first() hint to prefer index join
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
Tests